Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix #5411: Introduce asset download script [Blocked: #5416] #4885

Conversation

BenHenning
Copy link
Member

@BenHenning BenHenning commented Feb 28, 2023

Explanation

Fixes #5411

TODO: Finish.

Essential Checklist

  • The PR title and explanation each start with "Fix #bugnum: " (If this PR fixes part of an issue, prefix the title with "Fix part of #bugnum: ...".)
  • Any changes to scripts/assets files have their rationale included in the PR explanation.
  • The PR follows the style guide.
  • The PR does not contain any unnecessary code changes from Android Studio (reference).
  • The PR is made from a branch that's not called "develop" and is up-to-date with "develop".
  • The PR is assigned to the appropriate reviewers (reference).

For UI-specific PRs only

If your PR includes UI-related changes, then:

  • Add screenshots for portrait/landscape for both a tablet & phone of the before & after UI changes
  • For the screenshots above, include both English and pseudo-localized (RTL) screenshots (see RTL guide)
  • Add a video showing the full UX flow with a screen reader enabled (see accessibility guide)
  • Add a screenshot demonstrating that you ran affected Espresso tests locally & that they're passing

This is utilizing a new Oppia web API being worked on that will allow
for an easier collection of Oppia assets as part of a broader initiative
to fully automate Oppia Android's release pipeline.

There are a bunch of TODOs, documentation, testing, and functionality
work needed yet before this solution is ready to check in (including
introducing new proto to legacy proto conversion & outputting for the
app to actually be able to consume the lessons being imported from Oppia
web).

This also sets the groundwork for downloads infrastructure both for the
client & Oppia web's new proto APIs that will be introduced as part of
itnroducing lessond download support in the app.
@oppiabot
Copy link

oppiabot bot commented Mar 7, 2023

Hi @BenHenning, I'm going to mark this PR as stale because it hasn't had any updates for 7 days. If no further activity occurs within 7 days, it will be automatically closed so that others can take up the issue.
If you are still working on this PR, please make a follow-up commit within 3 days (and submit it for review, if applicable). Please also let us know if you are stuck so we can help you!

@oppiabot oppiabot bot added the stale Corresponds to items that haven't seen a recent update and may be automatically closed. label Mar 7, 2023
Specifically, adds support for:
- Embedding the API secret in the request header.
- Supporting a batch-API that allows multiple structures to be requested
simultaneouusly.

Note that while the script now supports batch retrieval, it doesn't
actually make use of it yet (since there's not an obvious way to test
this without production data, so this will be added later).
@oppiabot oppiabot bot removed the stale Corresponds to items that haven't seen a recent update and may be automatically closed. label Mar 14, 2023
@oppiabot
Copy link

oppiabot bot commented Mar 21, 2023

Hi @BenHenning, I'm going to mark this PR as stale because it hasn't had any updates for 7 days. If no further activity occurs within 7 days, it will be automatically closed so that others can take up the issue.
If you are still working on this PR, please make a follow-up commit within 3 days (and submit it for review, if applicable). Please also let us know if you are stuck so we can help you!

@oppiabot oppiabot bot added the stale Corresponds to items that haven't seen a recent update and may be automatically closed. label Mar 21, 2023
@oppiabot oppiabot bot closed this Mar 28, 2023
BenHenning and others added 13 commits May 31, 2023 00:30
Specifically:
- Temporarily disables version fallback (to detect issues with the
  script).
- Fixed multiple incompatibility issues, including adding a couple of
  temporary exemptions for the upcoming release. This also included
  fixing the invalid HTML tag regex pattern.
- Disabled classroom support since the production math classroom isn't
  yet available via the new classroom API.
- Added strong progress tracking for all expensive operations for a much
  easier time when using the script.
- Added on-disk server result caching for much faster subsequent runs
  (and to put much less stress on the remote server). This included
  adding support for converting downloaded structures back to JSON.
- Enabled image downloading.
- Added outputting proto v2 pb and textproto files (legacy proto
  conversion still needs to be added).
- Tuned parallelization to better fit hardware since otherwise a lot of
  strain is put on the OS's scheduler with the script's coroutine
  behaviors.
- Sped up some of the slower parts (JSON to proto conversion) of the
  script by better utilizing parallel coroutines.
- Added support for batch version fetching from remote, though it's
  currently disabled since there's an issue with using this:
  oppia/oppia#18241.
- Silence illegal reflection access warning caused by Retrofit and made
  other quality-of-life improvements to script output.

There's still more analysis required before this script can be
considered done from an alpha perspective, including:
- Double-checking constraint checking is working as expected (current
  reasoning suggests the language comprehension check isn't working).
- Adding legacy proto conversion & output support.
- Verifying that all failing responses from the emulated server are
  correctly failing.
- Verifying that the structures and images import correctly in the app.
This commit:
- Enables the language picker (being completed in #4762).
- Bumps version codes (x2 since an extra re-release of 0.10 was
  necessary, and the updated version codes for that release weren't
  checked in).
These strings come from learner studies as a backup to logging events to
Firebase. Local decoding is necessary to inspect and investigate the
logs locally.
Relevant link:
https://translatewiki.net/w/i.php?title=Special:ExportTranslations&language=pcm&group=oppia-android-app.

This fixes some of the spacing issues that were introduced by during
translation of the app strings.
This also adds support for Arabic in production builds (along with
Nigerian Pidgin), and fixes some issues with the Swahili configuration.

Nigeria was added as a new region for languages, including for
production.
These changes aren't release blockers but they've been irking me for a
while, so it's nice to get this output nicer. :)
Specifically:
- Follow-up fixes to add Naija support to the new language selector.
- Two post-merge nit fixes for the new language selector.
- Fixed locale selection by adjusting the matching algorithm and also
  moving the pcm translations to be NG-specific.

Some UI tests were removed that are particularly difficult to make pass,
and don't add a lot of value.
This enables spotlights specifically for alpha builds (via a new
alpha-only platform parameter module). This commit also adjusts
spotlights' overlay background color since it's a bit too dark as-is.

A new counter was added to count events during the app being open to
help track for lost events between two events that are received.
Specifically:
- Addressed failing static checks (including adding one test exemption
  for the new alpha-specific platform parameters module).
- Fixed lint issues.
- Added tests for the new debug event sequence number (and fixed
  existing tests for it).
- Added some notes in languages.proto on creole languages.
- Updated the profile selection logic in AndroidLocaleProfileFactory to
  more correctly prioritize Android language codes. This fixes language
  selection more in-line with the existing infrastructure than the
  previous solution, however it will require a large set of changes to
  AndroidLocaleProfileFactoryTest (which will happen in a follow-up
  commit).
- Removed all temporary TODOs that are either addressed, or are being
  tracked elsewhere. There are still some items that need to be
  addressed yet.

Note that Nigerian Pidgin has not yet been tested with content. Any
fixes necessary for that will be brought in via a follow-up commit.
As part of this, a few small issues were fixed in AndroidLocaleFactory
and its implementation was essentially recreated from the ground up to
better leverage code reuse (and thus gain more confidence in test
breadth since the actual contract of this factory is quite complex to
test comprehensively).
This removes some no-longer-needed comments from AndroidLocaleFactory.
It also updates the factory to be a bit more robust against missing
region definitions (rather than trying to proceed which could result in
an exception being thrown when it doesn't need to be).

This also fixes InitializeDefaultLocaleRule incorrectly creating an
empty region in cases when no region data is provided. This change fixes
the StateFragmentLocalTest failures reported by CI.
For some reason, fixing these 3 tests as part of the 5 that were failing
before is causing them to hang on Gradle (probably because they're
actually working correctly now, though it's not clear why it's hanging
vs. failing). Rather than investigating a potentially hard-to-track-down
issue, this just disables the tests in Gradle since Gradle is going away
soon, anyway, and the tests already run & pass with Bazel.
Some:
- Using an android_binary now so a deployable version of it can be built.
- Added a BUNCH of failure tolerance in multiple areas as part of getting
  things working "well enough" for the upcoming beta launch.
- Added a BUNCH of new analysis and metrics. Some of it's wrong, but a
  lot of it has correctly identified problems.
- Added conversion to legacy proto.
- Lifted the overwriting error for output files to make things a bit
  more streamlined.
Specifically:
- Disable Swahili again since it's not actually read for production
  launch.
- Fix audio language localization during audio selection.
- Fix a one-off crash that I noticed while changing languages and
  navigating.
- Added a built-in recovery mode to the decode event string utility to
  ensure that even truncated strings can at least be partially
  recovered.
This is only needed since the codegen doesn't seem to be working
entirely with Moshi 1.11 (which is what mainline is using, and it can't
upgrade to 1.13 without first upgrading Kotlin).
See previous commit for reasoning.
Add support for pinning topic list versions to allow for deterministic
asset downloads.
Copy link

oppiabot bot commented May 10, 2024

Hi @BenHenning, I'm going to mark this PR as stale because it hasn't had any updates for 7 days. If no further activity occurs within 7 days, it will be automatically closed so that others can take up the issue.
If you are still working on this PR, please make a follow-up commit within 3 days (and submit it for review, if applicable). Please also let us know if you are stuck so we can help you!

@oppiabot oppiabot bot added the stale Corresponds to items that haven't seen a recent update and may be automatically closed. label May 10, 2024
@adhiamboperes adhiamboperes removed the stale Corresponds to items that haven't seen a recent update and may be automatically closed. label May 15, 2024
Copy link

oppiabot bot commented May 22, 2024

Hi @BenHenning, I'm going to mark this PR as stale because it hasn't had any updates for 7 days. If no further activity occurs within 7 days, it will be automatically closed so that others can take up the issue.
If you are still working on this PR, please make a follow-up commit within 3 days (and submit it for review, if applicable). Please also let us know if you are stuck so we can help you!

@oppiabot oppiabot bot added the stale Corresponds to items that haven't seen a recent update and may be automatically closed. label May 22, 2024
BenHenning added a commit that referenced this pull request May 27, 2024
This pulls in the JSON-specific portions for the GAE Retrofit endpoint
configured in #4885 as part of a broader effort of splitting it into
multiple PRs.
…-download-script

Conflicts:
	third_party/maven_install.json
…troduce-asset-download-script

Conflicts:
	WORKSPACE
	data/src/main/java/org/oppia/android/data/backends/gae/model/GaeClassroom.kt
	scripts/src/java/org/oppia/android/scripts/gae/BUILD.bazel
	scripts/src/java/org/oppia/android/scripts/gae/GaeAndroidEndpoint.kt
	scripts/src/java/org/oppia/android/scripts/gae/GaeAndroidEndpointJsonImpl.kt
	scripts/src/java/org/oppia/android/scripts/gae/compat/TopicPackRepository.kt
	scripts/src/java/org/oppia/android/scripts/gae/gcs/GcsService.kt
	scripts/src/java/org/oppia/android/scripts/gae/json/AndroidActivityHandlerService.kt
	scripts/src/java/org/oppia/android/scripts/gae/json/AndroidActivityRequests.kt
	scripts/src/java/org/oppia/android/scripts/gae/json/GaeCustomizationArgValue.kt
	scripts/src/java/org/oppia/android/scripts/gae/json/GaeEntityTranslations.kt
	scripts/src/java/org/oppia/android/scripts/gae/json/MoshiFactory.kt
	scripts/src/java/org/oppia/android/scripts/gae/json/TypeResolutionContext.kt
	scripts/src/java/org/oppia/android/scripts/gae/proto/BUILD.bazel
	scripts/src/java/org/oppia/android/scripts/gae/proto/ImageDownloader.kt
	scripts/src/java/org/oppia/android/scripts/gae/proto/JsonToProtoConverter.kt
	scripts/src/java/org/oppia/android/scripts/gae/proto/LocalizationTracker.kt
	scripts/src/java/org/oppia/android/scripts/gae/proto/OppiaWebTranslationExtractor.kt
	third_party/BUILD.bazel

Note that most of these conflicts arose from formatting fixes in newly
split upstream PRs.
This undoes some Kotlin 1.5-specific things, plus one textproto
exemption change that's unrelated to the script.

It also puts in minimal multi-classroom support in order to build, but
the script is NOW BROKEN. This functionaltiy will need to be finished
before multiple classrooms will be fully supported.
@oppiabot oppiabot bot removed the stale Corresponds to items that haven't seen a recent update and may be automatically closed. label May 27, 2024
@BenHenning BenHenning changed the base branch from upgrade-to-kotlin1.6 to introduce-asset-script-gae-proto-endpoint-impl May 27, 2024 23:37
@BenHenning BenHenning changed the title Introduce asset download script [Blocked: #4937] Introduce asset download script [Blocked: #5416] May 27, 2024
@BenHenning BenHenning changed the title Introduce asset download script [Blocked: #5416] Fix #5411: Introduce asset download script [Blocked: #5416] May 27, 2024
BenHenning added a commit that referenced this pull request May 29, 2024
… for (multiple) classrooms & topic dependencies, and prepare for #4885 (#5398)

## Explanation
Fixes #1547
Fixes part of #169
Fixes part of #5344
Fixes part of #5365
Fixes part of #5411

The main purpose of this PR is to introduce support for multiple
classrooms in the data layer of the app (with minimal domain integration
to avoid the change extending beyond the lesson structures). However,
the PR is also doing a few more things including preparing the Android
codebase for introducing an asset download script (#4885) and other
peripheral cleanups of code (rather than updating it) that was noticed
along the way.

### Preparing for multiple classrooms

This addresses part of #5365.

#5344 is introducing support for multiple classrooms in the app. To help
prepare for those changes, this PR introduces the necessary data
structure and domain loading changes to load a new proto structure:

```proto
message ClassroomList {
  repeated ClassroomRecord classrooms = 1;
}
message ClassroomRecord {
  string id = 1;
  map<string, TranslationMapping> written_translations = 2;
  SubtitledHtml translatable_title = 3;
  LessonThumbnail classroom_thumbnail = 4;
  map<string, TopicIdList> topic_prerequisites = 5;
  message TopicIdList {
    repeated string topic_ids = 1;
  }
}
```

Rather than just a flat topic list. Some important details to note:
- The recommended topics structure has been updated to use this new
``topic_prerequisites`` value being loaded from classrooms. This will
also extend to production assets since the asset download script from
#4885 is also being updated to include support for this multiple
classrooms structure to address the remainder of #5365.
- To minimize domain changes, the new loading code assumes only **one**
classroom is present. TODOs have been added on #5344 to extend this to
support multiple classrooms.
- Current loading code (including for JSON) is ignoring all but:
``classrooms``, ``id``, and ``topic_prerequisites`` (including
``topic_ids``) from the protos above. These other fields are expected to
have supported added as part of #5344.
- There were some color simplifications made in ``TopicListController``.
These largely shouldn't have a major impact outside of developer assets.
These changes were made to ensure non-specificity to the previous lesson
classroom. In general, all of this should eventually be removed in favor
of loading colors from lesson assets, but that will need to wait until
the JSON pipeline is completely removed.

### Asset priming removal

This addresses part of #169.

``PrimeTopicAssetsController`` and its implementation were responsible
for hackily pre-loading all lesson images and audio to be on-device to
enable offline support. This was the first attempt at offline support
early in the app's development, but it had a few significant drawbacks:
- It required preloading everything upon first app open. Since it can
take a while for loading to occur, some robustness needed to be built in
for pausing, cancelling, and resuming. I'm not certain if these were
even 100% handled in the current implementation.
- It doesn't perform strong compatibility checks until you're in the app
which means lesson incompatibilities would just cause the app to get
stuck rather than addressing it during lesson import time (e.g. via an
asset downloader script).
- It required very significant workarounds to existing loading pipelines
that aren't ideal to keep in the codebase long-term (code smell).
- There's no guarantee the user even has enough disk space to download
all the needed assets (particularly audio), or if they'll have
sufficient internet connectivity & bandwidth to perform those downloads
upon first app open.

This approach was abandoned after the earliest alpha releases for an
asset download script (which is now being migrated over to this codebase
per #5411.

This removal unfortunately required removing a module that was
referenced in a lot of tests throughout the codebase. While the removal
itself was fairly simple, it does affect a lot of files.

Other areas changed (but unaffected by tests since these flows didn't
have automated tests):
- ``SplashActivityPresenter`` for enabling the downloader to start and
block the UI using a dialog box while the downloading occurred.
- ``AudioPlayerController` for removing the special loading logic for
primed audio files (the app now no longer supports loading audio files
from disk as we don't yet have a good long-term solution for
offline-available audio files due to their significant size).
- ``GlideImageLoader`` for removing support for loading locally cached
images (only through this flow; the flow we use for the asset download
script uses a different annotation and is still supported).

As alluded to above, two annotations were removed as part of this
cleanup:
- ``CacheAssetsLocally``: a boolean indicating whether the prime
download flow should be enabled.
- ``TopicListToCache``: this specified the list of topics to
pre-download if the flow was enabled.

### GAE structure cleanup & preparing for asset script

Preparing for #4885 included a few other changes, some of which were
nice-to-have:
- Introducing support for incorporating the protos from
https://github.com/oppia/oppia-proto-api (specifically
oppia/oppia-proto-api#1 since they are still a
work-in-progress).
- Adding ``java_proto`` versions for many of the app's proto structures
(since the download script runs in the JVM and doesn't use the javalite
proto environment).
- Removed all of the unused GAE services and models (addressing #1547 by
essentially making it obsolete), plus their mocks. These were never
hooked up, and they're never going to be: the long-term plan for the
codebase is to use new proto endpoints that will be added to Oppia web.
These Retrofit endpoints have actually been rebuilt and repurposed to be
used in the asset download script as a medium-term stop-gap until the
permanent proto endpoints can be added to Oppia web.
- ``RemoteAuthNetworkInterceptorTest`` was updated to use a different
example service since ``TopicService`` has been removed. The services
for platform parameters and user feedback are being kept.
- Some test file & KDoc exemptions have been removed since their
corresponding files have been deleted.

Note that the new Java proto targets & oppia-proto-api targets aren't
being used yet, but they will be in future PRs. This just provides the
basis of support for the asset download script while also helping to
split up the code to review across multiple PRs.

## Essential Checklist
- [x] The PR title and explanation each start with "Fix #bugnum: " (If
this PR fixes part of an issue, prefix the title with "Fix part of
#bugnum: ...".)
- [x] Any changes to
[scripts/assets](https://github.com/oppia/oppia-android/tree/develop/scripts/assets)
files have their rationale included in the PR explanation.
- [x] The PR follows the [style
guide](https://github.com/oppia/oppia-android/wiki/Coding-style-guide).
- [x] The PR does not contain any unnecessary code changes from Android
Studio
([reference](https://github.com/oppia/oppia-android/wiki/Guidance-on-submitting-a-PR#undo-unnecessary-changes)).
- [x] The PR is made from a branch that's **not** called "develop" and
is up-to-date with "develop".
- [x] The PR is **assigned** to the appropriate reviewers
([reference](https://github.com/oppia/oppia-android/wiki/Guidance-on-submitting-a-PR#clarification-regarding-assignees-and-reviewers-section)).

## For UI-specific PRs only
This is essentially only data infrastructural except for a couple of
points:
- Topic loading is now happening through a classrooms structure rather
than a tropic ID list. Since there's only one test & one production
classroom, this shouldn't actually change the UX.
- An at-app-open flow for predownloading image & audio assets has been
removed. This hasn't been used since the very earliest alpha releases of
the app, so it won't actually affect any users.
- Some colors for developer lesson and topic thumbnails were
updated--see above.
Copy link

oppiabot bot commented Jun 3, 2024

Hi @BenHenning, I'm going to mark this PR as stale because it hasn't had any updates for 7 days. If no further activity occurs within 7 days, it will be automatically closed so that others can take up the issue.
If you are still working on this PR, please make a follow-up commit within 3 days (and submit it for review, if applicable). Please also let us know if you are stuck so we can help you!

@oppiabot oppiabot bot added the stale Corresponds to items that haven't seen a recent update and may be automatically closed. label Jun 3, 2024
@oppiabot oppiabot bot closed this Jun 11, 2024
Note that this is slightly out-of-date with develop, so a follow-up
consolidation will be needed.

Additionally, this also makes effort toward allowing required language
enforcement to be re-enabled, but much more work is still needed here.
Certain content IDs have been silenced for failures due to
incompatibilities that will need to be fixed either in content or in the
import pipeline.

This also introduces support for caching latest versions of structures
which will help some with repeat performance (but it simplified other
aspects of topic pack construction).

Finally, a bunch of thoughts were added to DownloadLessons via a block
comment for future changes that should be considered as the download
script is finalized (to improve the script's robustness and
maintainability as lesson structures and requirements continue to change
over time).
…sset-download-script

Conflicts:
	WORKSPACE
	domain/BUILD.bazel
	domain/src/main/java/org/oppia/android/domain/topic/TopicListController.kt
	scripts/src/java/org/oppia/android/scripts/assets/DownloadLessons.kt
	scripts/src/java/org/oppia/android/scripts/assets/DtoProtoToLegacyProtoConverter.kt
	scripts/src/java/org/oppia/android/scripts/gae/GaeAndroidEndpointJsonImpl.kt
	scripts/src/java/org/oppia/android/scripts/gae/compat/TopicPackRepository.kt
	scripts/src/java/org/oppia/android/scripts/gae/proto/JsonToProtoConverter.kt
	scripts/src/java/org/oppia/android/scripts/gae/proto/LocalizationTracker.kt
Note that this + the previous merge likely introduced changes outside
the scope of the now-slimmer introduce-asset-download-script branch, and
such changes will eventually need to be split out to the correct
precursor branches in future commits.
@BenHenning BenHenning reopened this Jun 21, 2024
@oppiabot oppiabot bot removed the stale Corresponds to items that haven't seen a recent update and may be automatically closed. label Jun 21, 2024
@BenHenning BenHenning closed this Jun 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

GaeSolution's 'correct_answer' field should be 'Any?' instead of 'String?'
4 participants